[c++] Create C++ bindings for log filter pushdown - #4036
Conversation
naivedogger
changed the title
|
Thanks @leekeiabstraction for wiring batch statistics into the Rust client! This PR is now ready for review, @fresh-borzoni @charlesdong1991 @leekeiabstraction, appreciate a review when you have time 🙏 |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@naivedogger Thank you for the PR, left some quick comments, PTAL
Also don't forget to rebase, and ping me for another look, please 🙌
| PredicateLiteral(Time value); | ||
|
|
||
| static PredicateLiteral Null(); | ||
| static PredicateLiteral Decimal(std::string value); |
There was a problem hiding this comment.
Nothing tests the Decimal or Timestamp literals, including the check that rejects a literal the column's scale can't hold exactly.
Add a case for both, accepted and rejected?
| /// explicit factories below to preserve their Fluss logical type. | ||
| class PredicateLiteral { | ||
| public: | ||
| PredicateLiteral(bool value); |
There was a problem hiding this comment.
Equal(5) compiles but Equal(5L) and Equal(5u) are ambiguous across the five numeric ctors, I checked with clang.
Add overloads for long and unsigned, or make it a template?
|
|
||
| namespace { | ||
|
|
||
| enum class PredicateLeafFunction : int32_t { |
There was a problem hiding this comment.
This duplicates the Rust decoder's mapping, and only GreaterThan, StartsWith and And are covered by tests. Inserting an operator in either list mis-maps the rest silently.
cxx can generate the C++ enum from a shared enum in the bridge, so there'd be one list instead of two. function carries both leaf and compound codes today, so it needs splitting first.
d5d720b to
ecb216d
Compare
3 participants
Purpose
Linked issue: close #4035
Brief change log
Tests
API and Format
Documentation